Repository navigation
feat(reporting): notify the adopter when a reporting issue opens, escalates or resolves - #1875
Conversation
| plan: list[Migration] = [] | ||
| held = False | ||
| for migration in pending: | ||
| if migration.contract: |
There was a problem hiding this comment.
Medium: _past_held_contract (dry-run plan) and _migrate (apply) diverge on a second contract step. _migrate breaks on any non-additive past a held contract (if held and not migration.additive: break), so a second contract step stops the walk. Here a contract step only sets held = True and the loop continues, so an additive after the second contract is planned. For pending [0007c, 0008a, 0009c, 0010a] (all held): _migrate applies [0008], _past_held_contract returns [0008, 0010] — --dry-run/status reports work the real run won't do. Not reachable with today's single contract step (0007), so latent, but the two planners are written to agree and don't. Mirror _migrate: break when held and not migration.additive before testing migration.contract.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — additive issue-lifecycle event outbox + observer under feat(reporting):, correct non-breaking semver signal (new optional issue_observer, new exports, no public-surface removals or type changes).
What I checked
- New
issue_events.pymodules, memory store additions (store.pyclaim/lease/sweep/prune) — exactly-once semantics preserved. persist_replay_lifecycles_onrework: inserted/moved guards avoid double-recording — safe.0008SQL:UNIQUE(issue_id,kind)exactly-once,FOR UPDATE SKIP LOCKED+ NOT-EXISTS in-order claim — sound.- Dispatcher operability: async observer
asyncio.wait_fortimeout, per-event backoff/release, store failures routed toworker_error_handler. - Coverage: memory + PG conformance + type-check tests present.
Medium findings (1)
- packages/adcp-reporting-storage/src/adcp_reporting_storage/migrations/init.py:349 — Dry-run migration planner diverges from applier on a second contract step (latent; only 0007 exists today).
One medium finding, no critical/high, no escalation triggers (high_risk false, gated_paths false, no team gate, no prior decision). Below the three-medium threshold for comment → falls through to approve.
Medium findings
- packages/adcp-reporting-storage/src/adcp_reporting_storage/migrations/init.py:349 — Dry-run migration planner diverges from applier on a second contract step
b1aee14 to
43258f8
Compare
…sactional outbox Migration 0008 (header "-- adcp-upgrade: additive") adds reporting_issue_events and its indexes: one new table, nothing else, and the storage contract stays 2. Every write that moves a reporting_issue_lifecycle row (ensure opens, set_issue_state acknowledges or waives, the projection retires as resolved, replay persistence) records its transition there in the same transaction, so an event exists exactly when its transition committed. UNIQUE (issue_id, kind) keeps the record exactly once across replicas, and event_id is derived from (issue_id, kind), so every redelivery of a transition carries the same id. PgReportingLedgerStore gains the outbox the issue observer reads (adcp-client-python#1258): issue_events_supported and issue_events_ready(), sweep_issue_escalations(after=...) (settles each candidate account under its lock, then records 'escalated' for occurrences still open or acknowledged past opened_at + after, ON CONFLICT DO NOTHING), claim_issue_events (SKIP LOCKED leases, at most one event per occurrence, each occurrence in recording order), complete_issue_event, release_issue_event(retry_after=...) and prune_issue_events(older_than=...), both measured on the store's clock. ReportingIssueEventLease is exported. Until 0008 is applied, lifecycle writes record nothing and issue_events_ready() is False; nothing else changes. python -m adcp migrate now applies an additive migration even while a contract step waits, so 0008 lands on an expanded ledger beside adcp-reporting-storage 0.1.x/0.2.0b0 processes, which neither read nor write the table. The published-wheel rolling test runs them beside it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…alates or resolves ReliableReportingService(issue_observer=...) calls an adopter callback once per lifecycle transition of a reporting-issue occurrence: opened, escalated, acknowledged, waived, resolved (#1258). A CONSUMER_STATUS_MISMATCH is caller-scoped, so until now a seller only learned of one when the buyer read its status or called; an unattended statement was never noticed. The ledger records each transition in the same transaction as the lifecycle write (ingest, status reads, set_issue_state, retirement), exactly once across replicas. The worker turn sweeps escalations when opened_at + consumer_mismatch_escalation passes (settling the account first, so a cleared condition resolves instead of escalating), then delivers committed events to the observer: after commit, in recording order per occurrence, at least once. Every delivery of one transition carries the same event_id; observers deduplicate on it. An observer that raises or times out (10 s) never fails a buyer's call: it is reported to worker_error_handler as issue_observer:{event_id} and retried with backoff (30 s doubling to 1 h). ReportingIssueEvent carries event_id, kind, occurred_at, account_id, consumer_id, issue_id, attempt and the ReportingIssue as get_reporting_status rendered it for that caller at occurred_at (None for resolved). ReportingIssueEventDispatcher serves adopters who compose the ledger without the service. The in-memory store implements the ReportingIssueEventStore capability; PostgreSQL needs adcp-reporting-storage 0.2.0b4 with migration 0008, and the service refuses issue_observer with an actionable error otherwise. production= is refused with issue_observer for now (#1873). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
43258f8 to
fa4a30c
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — additive issue-observer outbox feature (#1258) with no blocking findings.
This is a subsequent review. The prior run flagged one medium finding (migrations/init.py:349 — dry-run planner diverging from applier on a second contract step). That finding is absent from the current run, indicating the developer addressed it. Net improvement.
What was checked:
- Feature is additive with correct semver and no wire change, so DR-0032/0033 pathway rules don't apply.
issue_events.py:327run_onceclaims leased batches and settles each lease;:399_backoffcaps retry atretry_cap.ledger/store.py:1921_record_issue_transitionis exactly-once per (issue_id,kind), no-ops on unchanged state;:1973sweep_issue_escalationssettles each account first.ledger/issue_events.py:61stable dedup id derivation.- At-least-once delivery, per-occurrence ordering, failing/hung-observer paths tested incl. two-replica PG conformance.
- Async observer timeout (10s via wait_for), worker-turn error isolation, escalation idempotency under concurrent sweeps, retention-bounded prune.
Gates: gated_paths is false; review_decision is APPROVED; no no-auto-approve team match; high_risk false.
Decision table: no critical/high/medium findings present (rows 1, 4–8 do not fire), gated_paths false (row 2 N/A), no deletions in high-risk paths (row 3 N/A). Falls through to row 9 → approve.
Non-blocking follow-up noted by reviewer: _snapshots adds a sibling resolved lease to unreadable on transient snapshot-read failure — a bounded, self-healing delay on the all-clear path, not data loss. Worth a glance but not actionable.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — additive issue-lifecycle event outbox and observer (#1258), purely additive public surface under feat: with the correct semver signal.
Gates checked:
- No no-auto-approve team match;
gated_paths: false;review_decision: APPROVED. high_risk: false.- No critical/high findings; no current medium findings.
Subsequent-review note: the prior Medium (dry-run migration planner diverging from the applier at migrations/__init__.py:349) is resolved — _past_held_contract now mirrors _migrate's held-contract walk, with a test pinning the second-contract-step case. Net improvement over the prior pass.
Reviewer verified: additive public surface (no wire/union/layering/credential concerns), CI gates intact, observer is a seller-side local callback (not a wire change, so DR-0032/0033 pathways don't apply), dispatcher lease sizing and lease_token/ON CONFLICT guards give correct at-least-once delivery, claim query enforces one-in-flight-per-occurrence across replicas, and the escalation sweep settles before escalating.
No blocking or medium findings remain — falls through to row 9.
Summary
Closes #1258. A seller can now learn, through a supported callback, when a reporting issue opens, escalates, is acknowledged or waived, or resolves. ESA can drop its
get_reporting_statusresponse scan.Stacked on the storage PR (#1874): until that merges, this diff also shows its commit. Merge #1874 first; this one merges after the adcp-reporting-storage beta release that ships migration 0008. Against a store without the capability, the SDK refuses
issue_observerwith an actionable error that namesadcp-reporting-storage 0.2.0b4.API
issue_observer: ReportingIssueObserveronReliableReportingService. It is any sync or async callable taking aReportingIssueEvent.ReportingIssueEvent(frozen) has:event_id: stable across redeliveries.kind:opened | escalated | acknowledged | waived | resolved.occurred_at,account_id,consumer_id,issue_id,attempt.issue: theReportingIssueasget_reporting_statusrendered it for that caller atoccurred_at, withresponsible_party,recommended_action,severity,opened_atand the causingreporting_status_id. It isNoneforresolved.reporting_status_id, a convenience accessor.ReportingIssueEventDispatcherfor adopters who compose the ledger without the service.ReportingIssueEventStore, the capability protocol, plussupports_issue_events().ReportingIssueEventLeaseandissue_event_id().ReliableReportingTurn.issue_eventsreportsescalations_recorded,deliveredandfailed.When it fires
set_issue_state, retirement and replay persistence. The record is exactly once across replicas.run_worker()orworker_intervaldelivers committed events. The same turn sweeps escalations. It settles the account first, so a cleared condition resolves instead of escalating, and an unattended statement escalates on time without anyone polling.event_id. Deduplicate on it. This is documented in the module docstring, the service docstring and the guide.worker_error_handlerasissue_observer:{event_id}.sync_reporting_status(unchanged) and repeated status reads record nothing.Scope decisions
production=. Leaving this out was agreed. The service raisesReliableReportingConfigurationErrorwhenproduction=andissue_observerare passed together. Follow-up: feat(reporting): support issue_observer on the production= service path #1873.occurred_at: statements recorded by then (chain leaf as of then), obligations and revisions created by then, and the occurrence in the event's state. Revision readability is mutable and has no history, so an event delivered after the seller changed readability can renderissue=None. Rendering is documented as best effort.@adcp/sdkhas no issue-level hook; its closest analog is the obligation-healthsubscribers/notifyoutbox. This design mirrors that model: a durable record, at-least-once delivery, a stable id, retry until success, a 10 s timeout. A follow-up asks TS forissueObserverwith the sameevent_idderivation: feat(reporting): issueObserver — notify the seller when a reporting issue opens, escalates or resolves (Python parity) adcp-client#3648.Tests
tests/test_reporting_issue_observer.py(memory):event_id,attempt=2production=, an unsupported store, a non-callable observer, a missing tableevent_idparity with the storage distributiontests/conformance/reporting/test_reporting_issue_observer_pg.pyruns the same flows onpostgres_storage, plus two replicas delivering each event once and refusal before migration 0008.tests/type_checks/reporting_issue_observer.py(mypy--strictand pyright).Docs: "Observe reporting issues" in
docs/reliable-reporting-service.md, and theworker_error_handlercomponent list.🤖 Generated with Claude Code